-
Notifications
You must be signed in to change notification settings - Fork 239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added support for brotli ('br') content-encoding #172
base: master
Are you sure you want to change the base?
Conversation
5dbc0cd
to
abed970
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure to update the documentation as well around it supporting br.
abed970
to
3d1cc0f
Compare
d67936f
to
6c91c94
Compare
@dougwilson There's an issue with |
I guess just pick a different module or an older version of that module. |
6c91c94
to
1cb1f12
Compare
d4a01cb
to
42dacd1
Compare
d3f283f
to
d557204
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment about the faster statement and still have an open question on how a user can change compression level of br.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment about the faster statement and still have an open question on how a user can change compression level of br. I'm also having trouble to actually get br compression to even work at all with Chrome. I'm trying to figure it out, as our number 1 issue opened here is how to get this module working, so having concrete information for how to get br working with a web browser (Chrome, for instance) would help a lot. For reference I used the example in the README and your branch and Chrome continues to only show it using gzip, even when the connection is https (which my understanding is a requirement for br to work in Chrome).
Look at Chrome's Or maybe it's because when it first arrived they considered it a good compression for WOFF fonts, but they always tested with the highest compression levels. Today people know that with level 4 you have better results on all kinds of files. |
@dougwilson I've removed the dependency on koa's modified code, and implemented a simple parsing function for the encoding, with built-in preferred-encoding support. this results in less code, and a faster code. Passing all old tests, new tests, and the latest ones added by @nicksrandall I really don't see anything blocking a merge now, and it would be great to have this in production :-) I've also rebased on |
6982dca
to
b024cce
Compare
I'm a bit concerned about why we should be adding such a large amount of code directly to this library, vs adding whatever is needed into Express.js's own Besides that, even just putting that code in here seems to have licensing issues, as it seems to be a large chunk of the |
This module uses I don't see a "large chunk" of Do you want to get |
It is part of Express.js: http://expressjs.com/en/resources/community.html#express-is-made-of-many-modules
Why not? I mean, just look at the person who is committing there... it is me. Are you saying I would not accept what I am suggesting you to put there?
Sure, you may have unrolled some of those functions together, but unless you are saying you definately didn't copy and paste from negotiator and just move around the code, then it is certainly the original license at play on that code. |
Good to know 😂
I do not really get the insistency on splitting everything to a million pieces, I don't see a value to any extremity, but on the other hand I don't mind making a PR to |
@dougwilson If we're at it - why did you filter out |
This is because when you combine the module's API and spec, that is the correct functionality. The API of the module only returns what is acceptable. The spec says that 0 is a special value of not acceptable (vs just the lowest acceptability, which would be 0.001). You can find the spec in RFC 7231.
|
@dougwilson you have a PR pending |
I guess everyone are in vacation now |
@danielgindi just want to say that this is good work. Thanks for pushing for this. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
At this point I'm convinced this is not going to happen. |
Would you like to create an npm package from your fork? Or should I? I really think it's much needed (and would like to use it myself) |
If it is helpful, I’ve built this package and am using it in production in several places: https://github.com/nicksrandall/compression |
This comment has been minimized.
This comment has been minimized.
Tempting, thanks for this. How has it been running, any issues? And this one that seems pretty active: https://github.com/Econify/compression-next#readme |
This comment was marked as abuse.
This comment was marked as abuse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Looks like the PR is approved, could you folks resolve the conflicts and merge? Is there anything else we're waiting on? |
Any plans on having this merged or is https://www.npmjs.com/package/shrink-ray-current the current best solution? |
Is there any current movement on getting this approved and merged? Im asking in reference to Apollo Server which we use for GraphQL. It doesn't support Brotli due to this dependency not supporting it. |
@dougwilson, I was curious if you have the bandwidth to review this PR and provide a secondary approval with @vinayakkulkarni having already reviewed and approved? Seeing Anything that the community can do to help see this PR land and become a reality? |
Realistically this repository should be considered abandoned, the maintainers haven't responded in a long time and brotli has been out for around 8 years so it's safe to assume there is no interest to add it. The community should probably rally around one of the multiple existing forks for this package. Maybe express-compression would be good? |
expressjs/body-parser#403
https://medium.com/oyotech/how-brotli-compression-gave-us-37-latency-improvement-14d41e50fee4
https://caniuse.com/#feat=brotli